Skip to content

Add fields property#1374

Merged
lzchen merged 9 commits intoopen-telemetry:masterfrom
ocelotl:issue_1104
Dec 2, 2020
Merged

Add fields property#1374
lzchen merged 9 commits intoopen-telemetry:masterfrom
ocelotl:issue_1104

Conversation

@ocelotl
Copy link
Copy Markdown
Contributor

@ocelotl ocelotl commented Nov 11, 2020 •
edited
Loading

Fixes #1104

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • test_fields

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

Tagging @carlosalberto as requested 👍

@ocelotl ocelotl requested review from a team, codeboten and hectorhdzg and removed request for a team November 11, 2020 01:43
@ocelotl ocelotl self-assigned this Nov 11, 2020
@ocelotl ocelotl added api Affects the API package. sdk Affects the SDK package. labels Nov 11, 2020
@ocelotl ocelotl force-pushed the issue_1104 branch 10 times, most recently from cba1c06 to 08d86fb Compare November 11, 2020 03:16
codeboten
codeboten suggested changes Nov 12, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking pretty good! Just a couple of questions I'd like to have answered before I approve

opentelemetry-api/src/opentelemetry/propagators/composite.py

for propagator in self._propagators:
for field in propagator.fields:
composite_fields.add(field)
Copy link
Copy Markdown
Contributor

@codeboten codeboten Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the behaviour for the composite propagator specified in the spec?

Copy link
Copy Markdown
Contributor Author

@ocelotl ocelotl Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is, I tried to replicate the Go implementation with one particular difference. In this implementation a set is returned instead of a list. There is no defined order for these fields (these fields may be called in inject in an arbitrary order and even if this order is constant it has no relation with the fields themselves), so an ordered sequence like a list makes no sense. Also, (if I understand correctly) the intention of this attribute is to allow the user to tell if a certain field is also used in inject. That is an in operation that has a better (O(1)) performance in a set than in a list (O(N)).

@lzchen lzchen self-assigned this Nov 12, 2020
@ocelotl ocelotl force-pushed the issue_1104 branch 2 times, most recently from 19ee9fa to 7d432b9 Compare November 13, 2020 04:50
@ocelotl ocelotl requested a review from codeboten November 13, 2020 05:09
@ocelotl ocelotl force-pushed the issue_1104 branch 2 times, most recently from 579af1a to 92ccbf1 Compare November 13, 2020 19:19
@codeboten codeboten assigned codeboten and unassigned ocelotl and lzchen Nov 19, 2020
codeboten
codeboten approved these changes Nov 24, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my comments.

@codeboten codeboten added the rc1 label Nov 25, 2020
@ocelotl ocelotl mentioned this pull request Nov 30, 2020
Closed
@ocelotl ocelotl force-pushed the issue_1104 branch 2 times, most recently from 1ff26a1 to c76a8f9 Compare December 1, 2020 00:23
NathanielRN
NathanielRN reviewed Dec 1, 2020
View reviewed changes
opentelemetry-api/CHANGELOG.md Outdated

## Unreleased

- Add `fields` to propagators ([#1374](https://github.com/open-telemetry/opentelemetry-python/pull/1374))
Copy link
Copy Markdown
Contributor

@NathanielRN NathanielRN Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR link should probably go on a new line

@NathanielRN NathanielRN mentioned this pull request Dec 2, 2020
Merged
11 tasks
@lzchen lzchen merged commit 50f5c6c into open-telemetry:master Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lzchen lzchen lzchen approved these changes

@hectorhdzg hectorhdzg Awaiting requested review from hectorhdzg

+2 more reviewers

@NathanielRN NathanielRN NathanielRN left review comments

@codeboten codeboten codeboten approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@codeboten codeboten

Labels

api Affects the API package. sdk Affects the SDK package.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Propagation: TextMap propagator must provide predefined propagation fields

4 participants

@ocelotl @codeboten @lzchen @NathanielRN